You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
streams: reject a direct source without pull() in the ReadableStream constructor - #48522
new ReadableStream({ type: "direct" }) accepts a source without pull. A direct stream never runs start(), so that source can never write.
A native sink (Bun.write, Bun.serve, fetch upload, Bun.spawn stdin, HTMLRewriter, S3) takes it as an empty body and leaves the stream readable and unlocked, so getReader().read() never settles afterwards.
The cause: the constructor stores a null pull (src/jsc/bindings/webcore/streams/JSReadableStream.cpp:344). Nothing else lets one in.
Fix
The constructor throws TypeError: The underlying source's 'pull' property is required when 'type' is "direct": a direct stream does not call start() for a direct source without pull.
The guards for a missing pull in readDirectStream and oneShotCallPull are dead and deleted.
Correct: no consumer gets such a stream, and the error comes before any I/O.
Verified: test/js/web/streams/streams.test.js (4 new cases fail on main). Self-reviewed: 25 concerns raised, 19 addressed (Notes).
Background
A type: "direct" stream gives pull() a controller that writes to the destination. A native sink calls pull() once.
Considered: fail at first use, after the sink truncated the file, sent the request or started the child.
Bun.serve answered 200 with an empty body for such a stream since 1.2.17. Now the handler throws and error() answers.
None found for a source with pull: release text and instruction counts checked (Notes).
Notes
Question for a maintainer: reject a missing pull at construction (this PR), or at first use?
I built and tested both. At first use, the owner of the sink gets the TypeError and the stream ends errored and locked, but the sink has acted by then:
A source without pull given to
Fails at first use
Constructor throws (this PR)
Bun.write over an existing file
rejects, the file is now empty
file intact
fetch POST body
rejects after the server handler ran once
no request
Bun.spawn stdin
throws, the child ran in 3 of 5 tries
no child
Bun.serve response
connection reset, error() not called
the handler throws, error() runs
The first-use variant keeps new ReadableStream({ type: "direct" }) constructible. It is about 9 lines in readDirectStream and I can switch to it.
Repro on main:
consts=newReadableStream({type: "direct"});awaitBun.write("/tmp/out.txt",newResponse(s));// 0s.locked;// falseawaits.getReader().read();// never settles
An S3 write of this stream sends a PUT with 0 bytes for the key.
Where this comes from. I found it by probing, there is no open issue. #18654 (closed, the crash it reports had another cause) attaches a Bun.serve program whose source has start(), cancel() and type: "direct" and no pull. Its log shows cancel() calls and no start() call. #18315 (closed) builds a direct source with only cancel() to show a cancel() bug. The repository has no other source like this: I scanned every type: "direct" literal in test, docs, src/js, packages and bench.
History of a source without pull (release binaries 1.2.17, 1.2.18, 1.3.0, 1.3.13, 1.3.14 and 1.4.0 to 1.4.4, Linux x64):
1.2.17
1.2.18 to 1.3.14
1.4.0
1.4.1 to main
getReader().read()
rejects
rejects
rejects
rejects
Bun.serve body
200, empty
200, empty
200, empty
200, empty
fetch upload
never settles
rejects
sends 0 bytes
sends 0 bytes
Bun.spawn stdin
not supported
child reads EOF
child reads EOF
child reads EOF
Bun.write(file, response)
never settles
never settles
never settles
resolves 0
HTMLRewriter
throws
throws
empty output
empty output
read() after Bun.write
rejects
rejects
rejects
never settles
Only Bun.serve has a stable empty body. The other sinks changed in 1.4.
Measurements. Release builds of the merge base 89452d2 and of this PR at 00f33d7. The commit after that one changes only the text of the TypeError (one string of 114 bytes). perf, valgrind and bloaty are not installed in the build container, so instructions come from perf_event_open (instructions:u), 5 interleaved runs per build.
Release text (size): 143,095,996 B on both builds. Linker map: the constructor +5 B, JSDirectStreamSource::finishCreation -7 B, consumeDirectStreamToArrayBuffer -108 B, assignToStream +81 B (the inliner now inlines JSPromise::create there). The TypeError string of oneShotCallPull is gone.
Instructions per new ReadableStream({ type: "direct", pull }): 1387.9 to 1382.4 (run spread 2.5, 2,000,000 runs).
Instructions per reader read(): 2233.8 to 2234.6 (spread 3.6 to 6.8, 2,000,000 reads). Per new Response(stream).bytes(): 13256.0 to 13238.2 (spread 15 to 33). Per HTMLRewriter attach: 11716.8 to 11708.0 (spread 30 to 68). All three are below the noise floor.
No new host function, allocation, or field.
Related open PRs.#46295 and #41561 add hasPull() for the two guards that this PR deletes. After this PR they drop it, and the test in #46295 that expects an empty body from Bun.write must expect the constructor TypeError. #42196 (draft) covers the sibling where the lazy start of a native stream fails.
Not changed here.
A pull() that throws synchronously into a native sink leaves the stream closed, not errored. The owner fails in both cases.
A direct source with start and pull constructs, and Bun never calls its start(). Should that be an error too?
callDirectPull keeps its TypeError for a controller without a source.
Self-review. It rejected the first version of this change (fail at first use, through one shared pull() method on JSDirectStreamSource) and asked for this one. Of its 25 concerns, 19 are addressed by the new shape and this text. The other 6 are about a synchronous pull() throw in readDirectStream (errored state, a falsy thrown value, two exits before the lock), which this PR does not touch.
Suites run. Debug ASAN build: streams.test.js (731 pass), serve-direct-readable-stream.test.ts (175), web-stream-state.test.ts (46), the spawn stdin suites, s3.test.ts (307), wpt-streams.test.ts (1175), html-rewriter.test.js (202), body.test.ts (776), node-stream.test.js (123), worker-terminate-lifetime.test.ts (27). The same files pass on the release build of this PR, with bun-write.test.js (104). On the release build of the merge base the 4 new cases fail.
no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/web/streams/streams.test.js
…constructor
A `type: "direct"` stream does not run start(), so a source without
pull() can never write or close. The constructor accepted it, and each
consumer gave it a meaning of its own: readers rejected with a
TypeError, and a native sink took it as a complete empty body and left
the stream readable and unlocked, so a later read() never settled.
The constructor now throws the TypeError that it already throws for a
pull that is not a function. The two guards for a missing pull in
readDirectStream and oneShotCallPull are unreachable and are deleted.
Reproduced on release builds of 1.4.4 and of main (89452d2):
consts=newReadableStream({type: "direct"});awaitBun.write("/tmp/out.txt",newResponse(s));// resolves 0s.locked;// falseawaits.getReader().read();// never settles
With this PR, the first line throws TypeError: The underlying source's 'pull' property is required when 'type' is "direct": a direct stream does not call start().
The 4 new cases in test/js/web/streams/streams.test.js fail on a release build of the merge base and pass on this branch.
No actionable comments were generated in the recent review.
Walkthrough
Direct-stream construction now throws a TypeError when the source lacks a callable pull. The implementation stores and invokes pull as required. Tests and documentation describe this behavior.
Changes
Direct stream pull requirement
Layer / File(s)
Summary
Validate and use the required pull method src/jsc/bindings/webcore/streams/JSReadableStream.cpp, src/jsc/bindings/webcore/streams/JSDirectStreamSource.h, src/jsc/bindings/webcore/streams/JSDirectStreamController.cpp, src/jsc/bindings/webcore/streams/BunStreamSource.cpp, src/jsc/bindings/webcore/streams/BunStreamConsumers.cpp, test/js/web/streams/streams.test.js, docs/runtime/streams.mdx, packages/bun-types/bun.d.ts
The constructor rejects direct sources without a callable pull. The source stores pull as required, and the read paths no longer handle its absence. Tests check sources without a callable pull. Documentation describes the requirement and states that direct streams do not call start().
Possibly related PRs
oven-sh/bun#41561: It changes direct-source pull() invocation and missing-pull handling.
oven-sh/bun#46295: It changes direct-stream pull() invocation and includes tests for missing pull. Both changes address the direct-stream pull contract.
Suggested reviewers: jarred-sumner
Merge Risk: Minimal · up to 5a381
Direct streams without pull now fail at construction as intended. No material merge risk is identified.
Pre-merge checks: 4
Check name
Status
Explanation
Title check
The title clearly and concisely states the main change: the constructor rejects a direct source without pull().
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Description check
The description explains the problem, the fix, and how the author verified it. It does not use the template’s exact headings, but it provides the information those sections require.
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.
The reason will be displayed to describe this comment to others. Learn more.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline note, I checked the deleted null-pull guards: the only other creator of JSDirectStreamSource (src/jsc/bindings/webcore/streams/BunAsyncIterableSource.cpp:546) passes the runtime's lazily-initialized asyncIterableSourcePull JSFunction, which is never null, and callDirectPull keeps its own null-source guard, so no remaining path reaches pullFunction() with an empty field. The dictionary conversion leaves dict.pull empty exactly when absent or undefined, so the new check and the four test variants line up. I did not run the test file; by reading, the base constructor passed nullptr and succeeded, so the new cases fail there. The construction-time versus first-use choice the description asks about is a maintainer call, and packages/bun-types/bun.d.ts has a CODEOWNER.
Extended reasoning...
The change makes the ReadableStream constructor reject a type: "direct" source without a callable pull, deletes the two now-unreachable null-pull guards in BunStreamConsumers.cpp and BunStreamSource.cpp, adds an ASSERT in JSDirectStreamSource::finishCreation, and updates the header comment, docs, .d.ts JSDoc, and a four-case it.each test asserting exact error name and message. It touches no security-sensitive surface. Deferring rather than approving because it is a user-visible tightening of a Bun extension that the author explicitly flags as needing a maintainer decision, and because the .d.ts file is CODEOWNER-owned.
A direct source without pull() gets its own TypeError text. The text
names the condition (type is "direct") and the reason (a direct stream
does not call start()). A pull that is present and not a function
keeps the existing text.
The reason will be displayed to describe this comment to others. Learn more.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked that the two deleted guards are genuinely unreachable: both JSDirectStreamSource::create callers now pass a non-null pull (the constructor after its new check, and BunAsyncIterableSource.cpp:546 with a runtime host function), the pull local in readDirectStream is still consumed at BunStreamSource.cpp:754, and the remaining null-check at JSDirectStreamController.cpp:720 guards a null source rather than a null pull. pull: null still routes through the existing "must be a function" error at JSReadableStream.cpp:179. I did not run the test file or BUN_JSC_validateExceptionChecks=1; this is from reading only.
Extended reasoning...
The change adds a construction-time TypeError for a type: "direct" source without pull, removes two downstream null guards, and updates the header comment, docs, .d.ts JSDoc and tests; it touches no security-sensitive surface. The .d.ts file is CODEOWNER-owned and the PR explicitly asks a maintainer to decide between rejecting at construction versus at first use, so a human decision is required regardless of the code being internally consistent.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Problem
new ReadableStream({ type: "direct" })accepts a source withoutpull. A direct stream never runsstart(), so that source can never write.Bun.write,Bun.serve,fetchupload,Bun.spawnstdin,HTMLRewriter, S3) takes it as an empty body and leaves the stream readable and unlocked, sogetReader().read()never settles afterwards.pull(src/jsc/bindings/webcore/streams/JSReadableStream.cpp:344). Nothing else lets one in.Fix
TypeError: The underlying source's 'pull' property is required when 'type' is "direct": a direct stream does not call start()for a direct source withoutpull.pullinreadDirectStreamandoneShotCallPullare dead and deleted.test/js/web/streams/streams.test.js(4 new cases fail on main). Self-reviewed: 25 concerns raised, 19 addressed (Notes).Background
type: "direct"stream givespull()a controller that writes to the destination. A native sink callspull()once.Downsides
pullnow throws at construction, even if the stream is only cancelled (thecancelcallback of "direct" readable stream doesn't get called #18315 repro).Bun.serveanswered 200 with an empty body for such a stream since 1.2.17. Now the handler throws anderror()answers.pull: release text and instruction counts checked (Notes).Notes
Question for a maintainer: reject a missing
pullat construction (this PR), or at first use?I built and tested both. At first use, the owner of the sink gets the TypeError and the stream ends errored and locked, but the sink has acted by then:
pullgiven toBun.writeover an existing filefetchPOST bodyBun.spawnstdinBun.serveresponseerror()not callederror()runsThe first-use variant keeps
new ReadableStream({ type: "direct" })constructible. It is about 9 lines inreadDirectStreamand I can switch to it.Repro on main:
An S3 write of this stream sends a PUT with 0 bytes for the key.
Where this comes from. I found it by probing, there is no open issue. #18654 (closed, the crash it reports had another cause) attaches a
Bun.serveprogram whose source hasstart(),cancel()andtype: "direct"and nopull. Its log showscancel()calls and nostart()call. #18315 (closed) builds a direct source with onlycancel()to show acancel()bug. The repository has no other source like this: I scanned everytype: "direct"literal intest,docs,src/js,packagesandbench.History of a source without
pull(release binaries 1.2.17, 1.2.18, 1.3.0, 1.3.13, 1.3.14 and 1.4.0 to 1.4.4, Linux x64):getReader().read()Bun.servebodyfetchuploadBun.spawnstdinBun.write(file, response)HTMLRewriterread()afterBun.writeOnly
Bun.servehas a stable empty body. The other sinks changed in 1.4.Measurements. Release builds of the merge base 89452d2 and of this PR at 00f33d7. The commit after that one changes only the text of the TypeError (one string of 114 bytes). perf, valgrind and bloaty are not installed in the build container, so instructions come from
perf_event_open(instructions:u), 5 interleaved runs per build.size): 143,095,996 B on both builds. Linker map: the constructor +5 B,JSDirectStreamSource::finishCreation-7 B,consumeDirectStreamToArrayBuffer-108 B,assignToStream+81 B (the inliner now inlinesJSPromise::createthere). The TypeError string ofoneShotCallPullis gone.new ReadableStream({ type: "direct", pull }): 1387.9 to 1382.4 (run spread 2.5, 2,000,000 runs).read(): 2233.8 to 2234.6 (spread 3.6 to 6.8, 2,000,000 reads). Pernew Response(stream).bytes(): 13256.0 to 13238.2 (spread 15 to 33). PerHTMLRewriterattach: 11716.8 to 11708.0 (spread 30 to 68). All three are below the noise floor.Related open PRs. #46295 and #41561 add
hasPull()for the two guards that this PR deletes. After this PR they drop it, and the test in #46295 that expects an empty body fromBun.writemust expect the constructor TypeError. #42196 (draft) covers the sibling where the lazy start of a native stream fails.Not changed here.
pull()that throws synchronously into a native sink leaves the stream closed, not errored. The owner fails in both cases.startandpullconstructs, and Bun never calls itsstart(). Should that be an error too?callDirectPullkeeps its TypeError for a controller without a source.Self-review. It rejected the first version of this change (fail at first use, through one shared
pull()method onJSDirectStreamSource) and asked for this one. Of its 25 concerns, 19 are addressed by the new shape and this text. The other 6 are about a synchronouspull()throw inreadDirectStream(errored state, a falsy thrown value, two exits before the lock), which this PR does not touch.Suites run. Debug ASAN build:
streams.test.js(731 pass),serve-direct-readable-stream.test.ts(175),web-stream-state.test.ts(46), the spawn stdin suites,s3.test.ts(307),wpt-streams.test.ts(1175),html-rewriter.test.js(202),body.test.ts(776),node-stream.test.js(123),worker-terminate-lifetime.test.ts(27). The same files pass on the release build of this PR, withbun-write.test.js(104). On the release build of the merge base the 4 new cases fail.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/web/streams/streams.test.js